Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

expr: stub protobuf support for BinaryFunc #11803

Merged
merged 2 commits into from
Apr 19, 2022

Conversation

aalexandrov
Copy link
Contributor

Adds partial protobuf support for BinaryFunc.

Motivation

  • This PR adds a known-desirable feature.

First step towards resolving MaterializeInc/database-issues#3431.

Tips for reviewer

The first commit partially implements Arbitrary for BinaryFunc.

  • We need a custom implementation because the one synthesized by the derive macro is hitting a known proptest issue1.
  • More arms need to be added as we build up the protobuf support for BinaryFunc towards closing MaterializeInc/database-issues#3431.

The third commit changes things as follows:

  • Adds ProtoBinaryFunc to func.proto. Variants that are not properly modeled as protobuf yet are prefixed with an // unsupported: comment.
  • Implements From<&BinaryFunc> for ProtoBinaryFunc in func.rs. Arms that are not yet supported have a todo!() placeholder.
  • Implements TryFrom<ProtoBinaryFunc> for BinaryFunc in func.rs. Arms that are not yet supported have a todo!() placeholder.

Testing

  • This PR has adequate test coverage / QA involvement has been duly considered.

Test coverage has been added for the variants that are already supported. As we add more variants, we will need to expand the custom Arbitrary implementation for BinaryFunc to keep this invariant.

Release notes

N/A

Footnotes

  1. https://github.com/AltSysrq/proptest/issues/152

@aalexandrov aalexandrov added C-feature Category: new feature or request A-compute Area: compute labels Apr 14, 2022
@aalexandrov aalexandrov self-assigned this Apr 14, 2022
src/expr/src/proto/scalar/func.proto Outdated Show resolved Hide resolved
src/expr/src/scalar/func.rs Outdated Show resolved Hide resolved
@aalexandrov aalexandrov changed the title expr: implement Arbitrary for BinaryFunc (incomplete) expr: add protobuf support for BinaryFunc (incomplete) Apr 15, 2022
@aalexandrov aalexandrov changed the title expr: add protobuf support for BinaryFunc (incomplete) expr: stub protobuf support for BinaryFunc Apr 15, 2022
@aalexandrov aalexandrov force-pushed the protobuf_binary_func branch 4 times, most recently from a4e9ced to 87aa8ab Compare April 15, 2022 13:35
- Add `ProtoBinaryFunc` to `func.proto`. Variants that are
  not properly modeled as protobuf yet are prefixed with an
  `// unsupported:` comment.
- Implement `From<&BinaryFunc> for ProtoBinaryFunc` in `func.rs`.
  Arms that are not yet supported have a `todo!()` placeholder.
- Implement `TryFrom<ProtoBinaryFunc> for BinaryFunc` in `func.rs`.
  Arms that are not yet supported have a `todo!()` placeholder.
@aalexandrov aalexandrov merged commit 229ecc1 into MaterializeInc:main Apr 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-compute Area: compute C-feature Category: new feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants